-
Notifications
You must be signed in to change notification settings - Fork 218
feat: contextual information about processing for loging using MDC #642
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This looks OK to me but I'm wondering how well it plays with Quarkus. |
No idea, next week will take some time and dig deep into quarkus side of things. |
Maybe we can merge and see, this how is works its just using ThreadLocals usually in background in logging frameworks. So I guess should not be a big deal. |
Maybe we can add this now, in worst case we create a revert commit, if there will be huge issues. |
Another thing is that I feel it's kind of redundant with the monitoring support. Granted, it's probably easier to use than seeing metrics and doesn't quite cover the same use case… |
Yes, this is related to log aggregation not to metrics. Like if you have logs in ELK stack and you want to see all logs related to a namespace or a resource. You can easily filter all the related logs. |
Actually will investigate how to have better correlation id support in a separate PR. To even better support log aggregation. |
LGTM, however I wonder if we should make such things something a user can opt in |
Hmm good question, so if the user should be able to specify the keys and values interested in. Wondering what would be the scope of that, just the custom resource? Doing the refactor on the other branch, so would rather merge this and create a separate issue and see if there is an elegant way to do it that way. |
I meant more a flag to enable/disable MDC entirely (or a dedicated module that enable MDC logging if on the classpath) |
@lburgazzoli what would be the benefit of that? |
There is usually a cost associated MDC (thread local, maps and other allocations) so if you don't require MDC, then you can turn it off and save some memory |
yes, but that part usually doable on the impl level, So not sure if it makes that much sense to solve on this level. In case the logger implementation would not handle that sure. |
I don't have any strong opinion, so it is fine with me but as personal preference, I always like more when I have the option to opt in to something that to opt out :) |
A completely agree in general. Want to do a round to review for v2 how we are configuring the operator and controller. Will take in mind also this and put it under a feature flag if doable nicely. |
closes #37